-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kubernetes-addon namespaces can be specified and creation optional #595
Conversation
51da248
to
1691a05
Compare
1691a05
to
fa86027
Compare
Please add |
fa86027
to
6ff42ee
Compare
Done @Hokwang. I thought the namespace had been removed entirely from the Terraform. Thanks for highlighting this. |
6ff42ee
to
4816040
Compare
4816040
to
f6ce234
Compare
@bryantbiggs any chance you could review this again? |
f6ce234
to
3fd6e73
Compare
3fd6e73
to
0805d7e
Compare
Thanks for the changes @bryantbiggs. Was just looking at your comments and then they appeared. |
@bobdoah 😅 give me a sec and I should have the rest of the updates - we'll get this across the line today |
d626d8f
to
04c6995
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @bobdoah !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobdoah Thanks for the PR but would expect that this PR should address only the namespace issue as mentioned the description.
This is a very big change with major refactoring for all the add-ons. It's advisable to do this kind of refactoring with small PRs for each add-on with proper testing.
module "helm_addon" { | ||
source = "../helm-addon" | ||
source = "../helm-addon" | ||
|
||
manage_via_gitops = var.manage_via_gitops | ||
helm_config = local.helm_config | ||
irsa_config = null | ||
addon_context = var.addon_context | ||
|
||
depends_on = [kubernetes_namespace_v1.this] | ||
} | ||
helm_config = merge( | ||
{ | ||
name = "agones" | ||
chart = "agones" | ||
repository = "https://agones.dev/chart/stable" | ||
version = "1.23.0" | ||
namespace = "agones-system" | ||
create_namespace = true | ||
description = "Agones Gaming Server Helm Chart deployment configuration" | ||
values = [file("${path.module}/values.yaml")] | ||
}, | ||
var.helm_config | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobdoah Any reason why you are refactoring all the add-ons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vara-bonthu I made most of the changes. When reviewing the PR, there were too many inconsistencies across the different addons in how names, nampespaces, etc. + the merging of provided config with var.helm_config
that I just dove in to start correcting. Please let me know how you would like to handle these changes so that the users can opt in or out of creating namespaces while keeping the addons consistent
04c6995
to
3f49a9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! pending CI checks.
@askulkarni2 should be ready to re-review if you have time - merge away if things look ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for all the changes to sort this @bryantbiggs and merging @askulkarni2 |
…on optional (aws-ia#595) Co-authored-by: Bryant Biggs <[email protected]>
What does this PR do?
This change makes it possible to use any of the kubernetes-addon modules with an already extant namespace. In a number of instances, the namespace creation is done via the IRSA module. In those cases, this PR also ensures the namespace is passed from the
helm_config
.Motivation
I'm attempting to get my colleagues to adopt this module into our EKS Terraform projects. In our case we deploy operational modules to a single namespace. This is created outside the addon modules. To migrate to these modules, we need to be able to disable namespace creation and allow the namespace to be specified.
More
pre-commit run -a
with this PRNote: Not all the PRs required examples and docs except a new pattern or add-on added.
For Moderators
Additional Notes